feat(core): Add extendIntegration method#21759
Conversation
size-limit report 📦
|
| options?: HttpServerSpansIntegrationOptions, | ||
| ) => Integration & { | ||
| name: 'HttpServerSpans'; | ||
| name: 'Http.ServerSpans'; |
There was a problem hiding this comment.
m: ist the name change intentional? (I like the new pattern but just wanted to flag)
There was a problem hiding this comment.
this was actually incorrect, if you look the actual name of the integration is Http.ServerSpans - one more reason why typing it this way is actually harmful. In v11 we should remove this cast probably and just let this be inferred.
| options?: HttpServerIntegrationOptions, | ||
| ) => Integration & { | ||
| name: 'HttpServer'; | ||
| name: 'Http.Server'; |
There was a problem hiding this comment.
m: ist the name change intentional? probably a fix for the INTEGRATION_NAME diversion
There was a problem hiding this comment.
same here, this just fixes the type to the actual name this has 😅
| const extendedBound = extendedValue.bind(wrappedIntegration) as typeof extendedValue; | ||
|
|
||
| const wrappedFunction = function (this: unknown, ...args: unknown[]): unknown { | ||
| baseBound(...args); |
There was a problem hiding this comment.
l: we could warn if we see that the return type of baseBound is not undefined
but yeah, as we discussed offline, it's not required to use this helper should we ever extend an integration with process* hooks, so not a blocker for me
| const wrappedFunction = function (this: unknown, ...args: unknown[]): unknown { | ||
| baseBound(...args); | ||
| return extendedBound(...args); | ||
| } as (Omit<Base, keyof Extended> & Extended)[Extract<keyof Extended, string>]; |
There was a problem hiding this comment.
nit: maybe this can be extracted as a type to make it more readable
There was a problem hiding this comment.
tried to make the types a bit easier to follow, it's a bit tricky and does not seem to work without some casting but hopefully it is clearer now!
9767b7b to
8b8480c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8b8480c. Configure here.
8b8480c to
2407b9e
Compare
2407b9e to
31520be
Compare

This adds a new method,
extendIntegration, that can be used to safely extend an integration.Today, we have the problem that if we extend an integration (e.g. node extending something from server-utils), we have to manually call the parent integration functions. This is a bit brittle because a) it requires you to know exactly what methods the parent integration has, which we usually want to abstract away from users on purpose. also, if a parent integration changes, this could be forgotten/lost.
Usage:
vs. before:
In addition, I also improved
defineIntegrationto maintain a static name, and made all integration names static for easier access.Note: We have a bunch of places where we started to type integrations manually, e.g.
We should revert this in v11. We purposefully did not do this because we want to keep the underlying implementation flexible. I think this was done sometimes to make it easier to extend/call things, but usually this should not be necessary and the extend helper should help with making things type safe a bit easier.